feat: Support Tool / Rule Filters in Task Frontmatter, Emit Task Frontmatter #88
Conversation
swedishborgie
force-pushed
the
main
branch
3 times, most recently
from
532e5ed to
f547a7b
Compare
Previous library is archived and is no longer maintained.
Changes: * Moved static path lists to paths.go * Removed global state for testability. * Split run() into multiple functions for readability / maintainability. * Added tests.
For Example:
```markdown
---
task_name: my_task
selectors:
language: go
env: prod
---
```
Would be the same as:
```
-s language=go -s env=prod
```
|
I agree with the "selectors". I think they should be a map rather than a list, selectors:
tool_name: [ gh, jira ] |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for task-level selectors to filter rules/tools, enables optional task frontmatter emission via the -t flag, and refactors main.go for improved testability by introducing a codingContext struct. The PR also migrates from gopkg.in/yaml.v3 to github.com/goccy/go-yaml for YAML parsing.
Key Changes
- Selector enhancement: Changed
selectorMapfrom simple string values to string slices, enabling OR logic when multiple values are specified for the same key - Task frontmatter selectors: Tasks can now define selectors in their frontmatter to filter which rules/tools are included
- Improved testability: Refactored
main.goby extracting acodingContextstruct with injected dependencies (output writers, command runner), enabling comprehensive unit testing
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| selector_map.go | Changed selectorMap implementation to use string slices for OR logic support, updated matching algorithm |
| selector_map_test.go | Added comprehensive test coverage for array selectors, OR logic, and mixed selector scenarios |
| main.go | Refactored into codingContext struct with extracted methods; added parseTaskFile to extract selectors from task frontmatter |
| main_test.go | Added extensive test suite (~70% coverage) with table-driven tests for all major functions |
| markdown.go | Removed parseMarkdownFileWithRawFrontmatter function, simplified to single parseMarkdownFile function; switched to goccy/go-yaml |
| remote.go | Extracted downloadRemoteDirectories and cleanupDownloadedDirectories methods into codingContext |
| paths.go | New file extracting path definitions into reusable functions (allTaskSearchPaths, allRulePaths, etc.) |
| go.mod / go.sum | Migrated from gopkg.in/yaml.v3 to github.com/goccy/go-yaml v1.18.0 |
|
@alexec -- Think this is ready for a final review. I've addressed all the copilot feedback and I've manually tested. This PR should be entirely backwards compatible. |
| func (includes *selectorMap) matchesIncludes(frontmatter frontMatter) bool { | ||
| for key, value := range *includes { | ||
| for key, values := range *includes { | ||
| fmValue, exists := frontmatter[key] | ||
| // If key exists, it must match the value | ||
| if exists && fmt.Sprint(fmValue) != value { | ||
| if !exists { | ||
| // If key doesn't exist in frontmatter, allow it | ||
| continue | ||
| } | ||
|
|
||
| // Check if frontmatter value matches any element in the inner map (OR logic) | ||
| fmStr := fmt.Sprint(fmValue) | ||
| if !values[fmStr] { | ||
| return false | ||
| } | ||
| // If key doesn't exist, allow it | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
[nitpick] The matchesIncludes function doesn't correctly handle empty value selectors (e.g., -s env=). According to the test cases, when a selector has an empty value and the key exists in frontmatter, it should not match. However, the current implementation doesn't check for this case.
When Set("env=") is called, it creates an empty map for the "env" key (line 47). Then in matchesIncludes, when the key exists in frontmatter (line 89), it converts the frontmatter value to string (line 95) and checks if it exists in the empty map (line 96). Since the map is empty, values[fmStr] will be false, which correctly returns false.
However, there's a logical inconsistency: an empty map map[string]bool{} means "match nothing", but the test expects it to mean "exclude files with this key". The current implementation happens to work due to the empty map behavior, but this is not semantically clear and could be confusing.
| case []any: | ||
| // Convert []any to map[string]bool | ||
| for _, item := range v { | ||
| cc.includes.SetValue(key, fmt.Sprint(item)) |
There was a problem hiding this comment.
[nitpick] The type conversion for []interface{} doesn't handle all possible YAML array element types. While fmt.Sprint(item) works for most cases, it may produce unexpected string representations for complex types (e.g., nested maps/arrays).
Consider adding explicit type checking for array elements or documenting that only scalar values are supported in selector arrays.
| cc.includes.SetValue(key, fmt.Sprint(item)) | |
| switch itemTyped := item.(type) { | |
| case string: | |
| cc.includes.SetValue(key, itemTyped) | |
| case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64, bool: | |
| cc.includes.SetValue(key, fmt.Sprint(itemTyped)) | |
| default: | |
| return fmt.Errorf("task file %s has invalid selector array value for key %q: expected scalar, got %T", cc.matchingTaskFile, key, item) | |
| } |
2 participants
This pull request adds the following features:
-sflag.-tflag.Example:
rule1.mdrule2.mdrule3.mdtask.mdWould select rule1, rule2.
Should do the same thing.
Setting an empty key (e.g.
-s rule=) will exclude all rules files with therulekey set.Non-functionally:
Split this into several different commits to hopefully make reviewing easier.